Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AUD-11] 애플 로그인 Redirection 구현 #3

Merged
merged 11 commits into from
Jan 21, 2024
Merged

Conversation

KimGaeun0806
Copy link
Member

📃 변경사항

  • 변수들 .env 파일로 이동
  • 애플로그인 링크 추가
  • 카카오 로그인과 애플로그인 쿼리들을 객체로 분리

🫨 고민한 부분

  • makeQueryString 함수의 config 파라미터의 타입을 어떻게 지정할지
  • loginConfig의 속성 일부를 as string으로 강제하는게 맞을지

💫 기타사항

image

  • 이렇게 말씀하신 부분에 대해서는 아직 저희 router 코드도 추가가 안되었고 백엔드와도 이야기가 되지 않아서 코드를 작성하지 않았습니다
    목요일 회의 이후에 작성하는 것이 좋을 것 같아서 두었는데 의견있으시면 말해주세요!

  • 그리고 애플은 잘 되는지 테스트를 못해봤습니다... 돈 나간다해서 ... ..
    이후에 따로 지라 이슈 만들어서 코드 완벽하게 추가하고 확인하는게 좋을 것 같아요

  • 코드는 나중에 파일 분리할 것 같습니다

  • 살짝 정신없는 상태로 코드를 작성햇네효,,,

@KimGaeun0806 KimGaeun0806 added the ✨ Feature 기능 개발 label Jan 17, 2024
@KimGaeun0806 KimGaeun0806 self-assigned this Jan 17, 2024
Copy link
Collaborator

@RookieAND RookieAND left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

애플 및 카카오 로그인 구현을 진행하시느라 고생 많으셨습니다.

코멘트에 추가로 고려해야 할 사항들과 Type Assertion 을 방지할 수 있는 방법에 대해 안내드렸습니다.
이를 수정하여 작업해주시면 감사하겠습니다.

.join('&');
};

const kakaoLoginLink = `https://kauth.kakao.com/oauth/authorize?${makeQueryString(kakaoLoginConfig)}`;
Copy link
Collaborator

@RookieAND RookieAND Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OAuth 구현을 위해 필요한 Redirect URL 은 constant로 관리해도 될 거 같습니다.
또한 각 플랫폼 별로 변수를 따로 두기보다는 Map 으로 관리하는 건 어떨까요?

예시로 작성한 코드는 아래와 같습니다.
꼭 하단의 코드를 참고하실 필요는 없고, 제가 생각했던 대략적인 구조에 대해서 말씀드리고자 제공된 코드이니 참고만 해주시면 감사하겠습니다.

    type SocialPlatformType = 'kakao' | 'apple';

    const socialLoginConstants = {
        kakao: {
            url: new URL(`https://kauth.kakao.com/oauth/authorize`),
            config: {
                client_id: import.meta.env.VITE_LOGIN_CLIENT_ID as string,
                redirect_uri: import.meta.env
                    .VITE_KAKAO_REDIRECTION_URL as string,
                response_type: 'code',
            },
        },
        apple: {
            url: new URL('https://appleid.apple.com/auth/authorize'),
            config: {
                client_id: import.meta.env.VITE_APPLE_CLIENT_ID as string,
                redirect_uri: import.meta.env.VITE_APPLE_REDIRECT_URL as string,
                response_type: 'code id_token',
                state: 'origin:web',
                scope: 'name email',
                response_mode: 'form_post',
                m: 11,
                v: '1.5.4',
            },
        },
    };

    const handleRedirectLogin = (platform: SocialPlatformType) => {
        const { url, config } = socialLoginConstants[platform];

        Object.entries(config).forEach(([option, value]) =>
            url.searchParams.set(option, String(value)),
        );

        window.location.href = url.toString();
    };

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이것도 좋은 방법인 것 같습니다.
일단은 대략적인 동작만 구현하려고 한 거라 상수 분리나 코드 정리하는 측면에서는 신경쓰지 않은 코드입니다
수정하도록 하겠습니다

const KAKAO_REST_API_KEY = ''; // TODO: 임시 KEY
const KAKAO_REDIRECT_URI = 'http://localhost:5173/redirect'; // TODO: 임시 URL
const kakaoLoginLink = `https://kauth.kakao.com/oauth/authorize?client_id=${KAKAO_REST_API_KEY}&redirect_uri=${KAKAO_REDIRECT_URI}&response_type=code`;
const appleLoginConfig = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 Config 도 컴포넌트 내부에서 관리되기 보다는 상수로 빼는게 좋아 보입니다.

};

const kakaoLoginConfig = {
client_id: import.meta.env.VITE_LOGIN_CLIENT_ID as string,
Copy link
Collaborator

@RookieAND RookieAND Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import.meta.env 는 기본적으로 환경 변수 타입에 undefined 유니온 타입을 붙이는데
이를 우회하려면 루트 디렉토리의 vite-env.d.ts 타입 정의 파일 내 ImportMetaEnv, ImportMeta 인터페이스 정의를 추가해야 합니다.

// vite-env.d.ts
/// <reference types="vite/client" />

interface ImportMetaEnv {
  readonly VITE_LOGIN_CLIENT_ID: string
}

해당 코드를 추가해주면 IDE 에서 정상적으로 타입을 string 으로 추론시킵니다.
참고 링크 : env-and-mode

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 그렇군요 참고하겠습니다 !

Base automatically changed from feature/t-bAUD-10-fe-카카오-로그인-redirect-기능-구현 to develop January 18, 2024 16:00
@KimGaeun0806
Copy link
Member Author

@RookieAND 저는 타입 정의할 때 항상 타입명 뒤에 Types을 붙여줬는데 (Ex. SocialLoginProviderTypes) 광인님은 어떻게 하시는 편인가요? 이것도 의견이 좀 갈려서 여기는 일단 Types를 빼두었습니다

@RookieAND
Copy link
Collaborator

저도 Type 을 뒤에 붙이는 편입니다.

Copy link
Collaborator

@RookieAND RookieAND left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정 작업 진행해주셔서 감사합니다 가은님.
몇 가지 코멘트를 하단에 남겨두었습니다.

return `${url}?${queryString}`;
};

const handleKakaoLogin = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도 함수를 각각 따로 만들기보다는 매개변수로 platform 을 받아서 붙이는 게 어떤가요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지금은 �함수가 비슷하지만 혹시나 이후 코드가 더 추가될까봐 일단은 분리해두었습니다..!
코드가 비슷할 것 같긴 하지만
만약에 코드 내용이 많이 달라진다면 굳이 함수를 하나로 만들 필요가 없을 것 같아서요

readonly VITE_KAKAO_REDIRECTION_URL: string;
readonly VITE_APPLE_CLIENT_ID: string;
readonly VITE_APPLE_REDIRECT_URL: string;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

첨부드린 공식문서에 나온 내용인데, 이걸 실제 ImportMeta interface 에 추가로 적용해주셔야 합니다.

interface ImportMetaEnv {
    readonly VITE_KAKAO_CLIENT_ID: string;
    readonly VITE_KAKAO_REDIRECTION_URL: string;
    readonly VITE_APPLE_CLIENT_ID: string;
    readonly VITE_APPLE_REDIRECT_URL: string;
}

interface ImportMeta {
  readonly env: ImportMetaEnv
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 아이고.. 그렇군요 수정하겠스빈다
공식문서를... 제대로 안읽은걸 들켰군요 ..

const kakaoLoginLink = `https://kauth.kakao.com/oauth/authorize?client_id=${KAKAO_REST_API_KEY}&redirect_uri=${KAKAO_REDIRECT_URI}&response_type=code`;
type SocialLoginProvider = 'apple' | 'kakao';

const socialLoginProvider = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

물론 지금은 기초 세팅을 위한 작업이지만 constants 폴더를 별도로 생성해서 해당 변수들을 옮기는 작업은 가능할 것 같습니다.

Copy link
Member Author

@KimGaeun0806 KimGaeun0806 Jan 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지금까지 객체 형태의 변수들은 따로 분리하지 않고 사용했었는데
분리하는 것이 더 깔끔할 수 있겠네요
constants/login.ts로 작성했는데 혹시 constants/login.contants.ts로 사용하시는 편인가요?
그 때 노션에 같이 작성했던 부분을 보면 아래와 같이 적혀있긴 한데 저 부분은 특정 컴포넌트에 귀속된 부분의 파일명인 것 같아서 여쭤봅니다

{component-name}/
	index.ts                      --- 파일 경로를 축약시키는 index.ts
	{ComponentName}.tsx           --- 컴포넌트 관련 로직 코드 작성
	{ComponentName}.css.ts        --- Vanilla Extract 코드 작성
	{ComponentName}.constants.ts  --- 컴포넌트에 귀속된 상수 목록
	{ComponentName}.util.ts       --- 컴포넌트에 귀속된 유틸 로직

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 경우에는 Component 에 종속시키기보다는 로그인이라는 하나의 핵심 기능이기 때문에 /constants 폴더에 값을 두고 사용하는 것이 더 낫지 않나 싶어서 제안드렸습니다. 만약 넣는다면 /constants/oauth.ts 여도 좋을 거 같네요

상단의 컨벤션은 특정 컴포넌트 에 종속된 상수를 관리하는 방법이기 때문에 /constants 폴더와는 약간의 차이가 있다고 생각합니다.

@KimGaeun0806
Copy link
Member Author

저도 Type 을 뒤에 붙이는 편입니다.

그럼 저도 그렇게 통일하도록 하겠습니다! 컨벤션에도 추가하도록 할까요?

@RookieAND
Copy link
Collaborator

@KimGaeun0806 넵, 컨벤션에도 Type 관련 내용 추가 부탁드립니다.

@RookieAND RookieAND self-requested a review January 21, 2024 13:50
Copy link
Collaborator

@RookieAND RookieAND left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정 사항 모두 반영된 것을 확인했습니다. 고생 많으셨습니다.
App.tsx 충돌난 사항은 더 이상 사용하지 않는 파일이니 제거한 후 commit & push 부탁드립니다.

@KimGaeun0806 KimGaeun0806 merged commit 5d4712b into develop Jan 21, 2024
1 check passed
@KimGaeun0806 KimGaeun0806 deleted the feature/AUD-11 branch January 21, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Feature 기능 개발
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants